Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[async-await] Added generation of interceptors #1272

Merged

Conversation

patskovn
Copy link
Contributor

Currently interceptors does not propagated to calls. Seems like it was forgotten to be implemented in this alpha implementation.

Currently interceptors does not propagated to calls
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 19, 2021

CLA Signed

The committers are authorized under a signed CLA.

@Lukasa
Copy link
Collaborator

Lukasa commented Sep 20, 2021

Thanks for this! Can you regenerate the various grpc examples? make generate-echo generate-helloworld generate-route-guide generate-normalization should do it.

@patskovn
Copy link
Contributor Author

Thanks for this! Can you regenerate the various grpc examples? make generate-echo generate-helloworld generate-route-guide generate-normalization should do it.

Done. Async flag set only for echo generation, but I still launch whole generation. It added additional newline to the bottom of files. Not sure is it ok or not :)

@Lukasa
Copy link
Collaborator

Lukasa commented Sep 20, 2021

The extra newlines are fine 😄

@Lukasa
Copy link
Collaborator

Lukasa commented Sep 20, 2021

Do you feel up to writing some tests? I'd love to see a couple of new tests, probably in InterceptorsTests, that validate that the interceptors continue to work correctly in async/await land.

@patskovn
Copy link
Contributor Author

I decided to create separate test class for async interceptors in analogy with other async tests like GRPCAsyncClientCallTests. I called file InterceptorsAsyncTests so that its be close to default counterpart InterceptorsTests.
Also, writing tests directly in current InterceptorsTests file will cause it suffer from lots of ifdefs and @availables

Waiting for further improvements suggestion 👌

@patskovn
Copy link
Contributor Author

Hello, @Lukasa! What are next steps to make further progress in this PR? Should I wait more for your review when you will have time or should I improve it somehow?

@glbrntt glbrntt self-requested a review September 27, 2021 12:56
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Sep 27, 2021
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops..! Thanks for catching and fixing this @Nekitosss. Would you mind running ./scripts/format.sh to make the CI happy?

Tests/GRPCTests/InterceptorsAsyncTests.swift Outdated Show resolved Hide resolved
Tests/GRPCTests/InterceptorsAsyncTests.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @Nekitosss 🙏

@glbrntt glbrntt merged commit b545bdd into grpc:1.4.1-async-await Sep 29, 2021
glbrntt pushed a commit that referenced this pull request Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants